Codecov Report

Attention: Patch coverage is 86.17363% with 86 lines in your changes missing coverage. Please review.

Project coverage is 70.13%. Comparing base (9720b46) to head (54419cf).

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
+ Coverage   70.01%   70.13%   +0.12%     
==========================================
  Files         380      385       +5     
  Lines       31816    31962     +146     
  Branches     4455     4490      +35     
==========================================
+ Hits        22275    22418     +143     
+ Misses       8508     8500       -8     
- Partials     1033     1044      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

Fixes #266

The only outstanding bullet point from the original description that I think I haven't covered is: "NParallelTextCorpora will take a parameter to determine how equivalent references are combined: choose first, combine all, or permutate all". Is this something we'll really want? Or is there a behavior we'll always prefer? Right now it's permutating.

-- commits line 65 at r1:
Can you clean up this history?

src/SIL.Machine/Corpora/CorporaExtensions.cs line 577 at r1 (raw file):

            public override IEnumerable<TextRow> GetRows(IEnumerable<string> textIds)
            {
                int indexOfInRangeRow = -1;

Can you explain in English what the indexOfInRangeRow is trying to accomplish?

src/SIL.Machine/Corpora/CorporaExtensions.cs line 586 at r1 (raw file):

                        .ToList();
                    IReadOnlyList<int> indices =
                        nonEmptyIndices.Count > 0 ? nonEmptyIndices : Enumerable.Range(0, nRow.N).ToList();

How does this logic work with AllRows? Are they blank rows being send down?

src/SIL.Machine/Corpora/CorporaExtensions.cs line 586 at r1 (raw file):

Could you elaborate on the connection with allRows? Yes, there may be empty rows output if all the rows available are empty. But it would be an unusual circunstance.

src/SIL.Machine/Corpora/CorporaExtensions.cs line 577 at r1 (raw file):

The point of the variable is to keep track of whether you previously picked a row from a particular source that was the beginning of a range. This way, you'll keep picking from that source until the range is finished.

-- commits line 65 at r1:

Sure! Would it be simpler just to squash this when I'm merging/rebasing?

src/SIL.Machine/Corpora/NParallelTextCorpus.cs line 222 at r1 (raw file):

                    {
                        int compareAlignmentCorpus = -1;
                        if (AlignmentCorpus != null)

What exactly is the function of the alignment corpus? Why is it active here, but not above when not all corpora were in lock-step?

src/SIL.Machine/Corpora/NParallelTextCorpus.cs line 250 at r1 (raw file):

                                    && minRefIndexes.All(j => j == i || !AllRowsList[j])
                                )
                                .Any(b => b)

Many of these are cryptic logic. Let's have at least a sentence per set of logic. "doing it the right way" would likely involve creating a dozen or so sub-functions to do the intermediate processing, but that may lead to more confusion. I just want to have enough bread crumbs that when someone comes back in 3 years, they'll be able to understand what is going on here without having to spend 3 days.

src/SIL.Machine/Corpora/NParallelTextCorpus.cs line 250 at r1 (raw file):

Or, if you take the complicated logic in these if statements and make functions with descriptive names, like below with "rangeInfo.IsInRange && AllInRangeHaveSegments(currentIncompleteRows)". That is decodable - the above logic less so.

src/SIL.Machine/Corpora/NParallelTextCorpus.cs line 345 at r1 (raw file):

            object[] defaultRefs = new object[] { };
            if (rows.Any(r => r != null))

Why rows.Any if you already threw a Argument null exception above?

Once Damien's concerns are resolved, I'm fine with the changes.

src/SIL.Machine/Corpora/CorporaExtensions.cs line 541 at r2 (raw file):

Let me see - "AllRows" is valuable when wanting to align to VRef.txt - so you get one line per vref entry (needed for AQUA as it relies on that specific format). Beyond that, I can't think of any need for "AllRows". And that should likely be it's own function, taking a single TextCorpus and then calling "AlignToVref" which would return a single TextCorpus with 1 entry per VRef. Therefore, it would not be needed when "merging" or "choosing random" but merely in adding another extension to format a single existing corpus. @ddaspit, does this sound right to you?

src/SIL.Machine/Corpora/CorporaExtensions.cs line 541 at r2 (raw file):

Actually, should we remove "AllRows" entirely from NParallelCorpus and rather just have a separate function that takes a single ITextCorpus to "AlignToVref"?

src/SIL.Machine/Corpora/CorporaExtensions.cs line 541 at r2 (raw file):

We could also just rip out "AllRows" right now and then add the new function later, as it is not needed now.

Actually, should there be an ability to filter out non-scripture? What is the best way to do that?

src/SIL.Machine/Corpora/CorporaExtensions.cs line 541 at r2 (raw file):

I think that AllRows is generally useful not just for us but perhaps even for others using the Machine library. One place we need it on the Serval side is when we align the pretranslate source and the train target: Since many rows will not have a target, this is necessary. Also, we use it currently (see the other PR) when doing the AlignManys since each corpus has already been filtered by textId/scriptureRange, meaning that in many cases, they won't overlap.

@johnml1135, it's saying I can't merge without your approval. I tried to dismiss you from the discussion because I think your question was answered, but it's still not letting me. If you could take a look when you have time, I'd appreciate it :)